HOTFIX: 2D .CSV Function and missing set_get_value_opt call #478
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## hotfix/v1.1.1 #478 +/- ##
================================================
Coverage ? 70.87%
================================================
Files ? 55
Lines ? 9236
Branches ? 0
================================================
Hits ? 6546
Misses ? 2690
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| if isinstance(source, (list, np.ndarray, str, Path)): | ||
| # Deal with csv or txt | ||
| if isinstance(source, (str, Path)): | ||
| # Convert to numpy array | ||
| source = np.loadtxt(source, delimiter=",", dtype=float) | ||
|
|
There was a problem hiding this comment.
I understand that this is necessary in order to check the validity of the inputs/outputs, but it is quite strange to read the csv here, throw away the result, and read it again in set_source.
There was a problem hiding this comment.
I would say that loading it twice is really not a good idea. It can be a huge problem if the CSV file is large and takes a while to read.
Furthermore, I would not recommend changing this in this hotfix PR. It doesn't exactly fix a bug.
There was a problem hiding this comment.
I understand that this is necessary in order to check the validity of the inputs/outputs, but it is quite strange to read the csv here, throw away the result, and read it again in
set_source.
Yep, it is pretty strange, but the whole _check_user_input is already strange. It is called twice (and I am not sure but depending on the source type it might be called 3 times) during the __init__. Also it has to process the source to be able to check if it is a valid input, but it does not save that processing, the saving is left to be dealt in set_source where it has to do all the calculations again. The source is processed twice for sources of type list and np.array as well
And if the source is anything besides (list, np.ndarray, str, Path) this _check_user_input method does essentially nothing
Also _check_user_input is a static_method for some reason?
What I am getting at is that all the user's inputs arguments should not be checked inside the same function, but rather in the calls of the set methods for those attributes. Maybe even inside .setter decorators. Structure wise, this method does not really work for the Function class, but this is a only a hotfix pr, we can focus on improving the method later
Furthermore, I would not recommend changing this in this hotfix PR. It doesn't exactly fix a bug.
This str and Path check is half of the bug corrections in this PR. Without these lines 2D .csv Functions will not have interpolation
There was a problem hiding this comment.
Indeed, in retrospect, it is a bit odd to perform the same operations multiple times in check inputs and while setting the source.
I approved the PR, since I don't think changes to the check method is in the scope of a hotfix. The PR fixes the issue with csv_2d. In a future PR the check inputs could be made as a way of pre-processing the source so as to only return a np.array or a callable to the set_source.
This would decouple responsabilities a fair bit and could be implemented in various ways, maybe with functools.singledispatch.
| if isinstance(source, (list, np.ndarray)): | ||
| if isinstance(source, (list, np.ndarray, str, Path)): | ||
| # Deal with csv or txt | ||
| if isinstance(source, (str, Path)): |
There was a problem hiding this comment.
I believe a try-except with FileNotFound would be beneficial here, just because it is a good practice. But again, this is also done in set_source.
| if isinstance(source, (list, np.ndarray, str, Path)): | ||
| # Deal with csv or txt | ||
| if isinstance(source, (str, Path)): | ||
| # Convert to numpy array | ||
| source = np.loadtxt(source, delimiter=",", dtype=float) | ||
|
|
There was a problem hiding this comment.
I would say that loading it twice is really not a good idea. It can be a huge problem if the CSV file is large and takes a while to read.
Furthermore, I would not recommend changing this in this hotfix PR. It doesn't exactly fix a bug.
Pull request type
Checklist
black rocketpy/ tests/) has passed locallypytest --runslow) have passed locallyCurrent behavior
If you create a Function based on a 2D .csv file, it will not work properly due to the interpolation being set to None. It also has no
get_value_optdefinedNew behavior
2D .csv based Functions now work as expected.
Also, tests checking the issues here have beend added
Breaking change